OLS-3126: Set LIGHTSPEED_AGENT_PROVIDER on sandbox templates#13
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@onmete: This pull request references OLS-3126 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
onmete
force-pushed
the
ols-3126-sandbox-provider-env
branch
from
ca44e0a to
ecff357
Compare
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
onmete
force-pushed
the
ols-3126-sandbox-provider-env
branch
2 times, most recently
from
41fc923 to
6718b32
Compare
|
|
||
| func vertexModelEnvVar(mp agenticv1alpha1.GoogleCloudVertexModelProvider) string { | ||
| switch mp { | ||
| case agenticv1alpha1.GoogleCloudVertexModelProviderAnthropic: |
There was a problem hiding this comment.
Why are we 3 different variable names for different providers just to map it back to single value in sandbox router? https://github.com/openshift/lightspeed-agentic-sandbox/blob/578aa4b5931571bc2eac05580e9faff09630887c/src/lightspeed_agentic/routes/__init__.py#L32
The only reason I've found was setting the default model, which could be simplified bu just setting the default based on the provider. Exposing 3 different vars for the same thing here feels like a leaky abtraction. IMO it would be better for the sandbox just to get MODEL_PROVIDER and MODEL, and doing the rest inside sandbox codebase.
| if err := setEnvVar(tmpl, "CLAUDE_CODE_USE_VERTEX", "1"); err != nil { | ||
| return fmt.Errorf("set CLAUDE_CODE_USE_VERTEX: %w", err) | ||
| if cfg.ModelProvider == agenticv1alpha1.GoogleCloudVertexModelProviderAnthropic { | ||
| if err := setEnvVar(tmpl, "CLAUDE_CODE_USE_VERTEX", "1"); err != nil { |
There was a problem hiding this comment.
Does the operator really need to know about CLAUDE_CODE_USE_VERTEX. This feels like sandbox-specifc thing. Doing it here creates high coupling beween the specific sandbox implementation and the operator (that IMO should not care how exactly the sandbox is setup)
onmete
force-pushed
the
ols-3126-sandbox-provider-env
branch
from
6718b32 to
a725d97
Compare
|
Warning Review limit reached
More reviews will be available in 54 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Map LLMProvider type to sandbox SDK runtime and set the matching model env var. For GoogleCloudVertex, use the new modelProvider field (from OLS-3150) to route to the correct SDK: Anthropic → claude (ANTHROPIC_MODEL, CLAUDE_CODE_USE_VERTEX=1) Google → gemini (GEMINI_MODEL) OpenAI → openai (OPENAI_MODEL) Other providers are unchanged: Anthropic/AWSBedrock → claude, OpenAI/AzureOpenAI → openai. Fix: OPENAI_MODEL for OpenAI/AzureOpenAI (was always ANTHROPIC_MODEL). Co-authored-by: Cursor <cursoragent@cursor.com>
onmete
force-pushed
the
ols-3126-sandbox-provider-env
branch
from
a725d97 to
5139443
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@onmete: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
LIGHTSPEED_AGENT_PROVIDERon derived sandbox templates fromLLMProvider.spec.typeGoogleCloudVertex, use the newmodelProviderfield (from OLS-3150 / PR OLS-3150: Add googleCloudVertex.modelProvider to LLMProvider CRD #14) to route to the correct SDKOPENAI_MODELfor OpenAI/AzureOpenAI (was alwaysANTHROPIC_MODEL)Depends on
googleCloudVertex.modelProvider)Mapping
Unblocks sandbox
/readyreadiness checks (OLS-3060) and correct OpenAI/Gemini SDK selection in production.Related
Test plan
make test(sandbox_templates tests for all provider types including Vertex Anthropic, Google, OpenAI)Made with Cursor